Merge branch dev with rel-10.3 - #25298
Merged
Merged
Conversation
When IsEnabledAsync(TState[]) evaluates non-batch checkers, the original implementation called InternalIsEnabledAsync for each state, which created a new DI scope every time. In real-world scenarios with thousands of permissions (e.g. 4050 permissions each with RequireFeaturesSimpleStateChecker), this caused N scope creations and N redundant Redis queries, resulting in ~3.5s latency. This change shares a single DI scope across all non-batch checker evaluations in the batch path by extracting EvaluateCheckersAsync and calling it directly with the shared scope. Each state still gets an isolated ITransientCachedServiceProvider to prevent transient service leakage across states, while scoped services (e.g. IFeatureChecker) are naturally shared within the scope, enabling cache reuse. The single-state path (IsEnabledAsync(TState)) remains completely unchanged.
Replace linear scans with Dictionary/HashSet lookups in both RequirePermissionsSimpleBatchStateChecker and RequireFeaturesSimpleBatchStateChecker for consistency.
…er-shared-scope Improve batch state checker performance and add RequireFeaturesSimpleBatchStateChecker
voloagent
marked this pull request as ready for review
April 20, 2026 11:47
voloagent
approved these changes
Apr 20, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This automated merge brings the dev branch changes into rel-10.3, primarily introducing batch feature-checking support for simple state checkers (mirroring existing batch permission checking) and refining SimpleStateCheckerManager’s batching behavior and DI-scope usage for performance and correctness.
Changes:
- Add
RequireFeaturesSimpleBatchStateChecker(plus model + tests) and extendIFeatureCheckerwith a batch API to check multiple features at once. - Update UI menu/toolbar building to enable batch feature checking via ambient
Use(...)scopes and add required module/project dependencies. - Refine
SimpleStateCheckerManagerbatch evaluation to reuse a single DI scope and add regression tests for scope/isolation behavior.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/permission-management/test/Volo.Abp.PermissionManagement.Domain.Tests/Volo/Abp/PermissionManagement/PermissionDefinitionSerializer_Tests.cs | Updates tests to call the new RequireFeatures overload explicitly controlling batchCheck. |
| framework/test/Volo.Abp.Features.Tests/Volo/Abp/Features/RequireFeaturesSimpleBatchStateChecker_Tests.cs | New tests validating feature batch checker behavior and ambient switching via Current/Use. |
| framework/test/Volo.Abp.Core.Tests/Volo/Abp/SimpleStateChecking/SimpleStateChecker_ScopeIsolation_Tests.cs | New tests probing scope reuse and cached provider isolation behavior. |
| framework/test/Volo.Abp.Core.Tests/Volo/Abp/SimpleStateChecking/SimpleStateChecker_BatchSingleScope_Tests.cs | New tests ensuring batch evaluation correctness when reusing a single DI scope. |
| framework/src/Volo.Abp.UI.Navigation/Volo/Abp/Ui/Navigation/MenuManager.cs | Wraps menu configuration in RequireFeaturesSimpleBatchStateChecker.Use(...) to support batch feature checks. |
| framework/src/Volo.Abp.UI.Navigation/Volo/Abp/Ui/Navigation/AbpUiNavigationModule.cs | Adds AbpFeaturesModule dependency so feature checking is available in navigation. |
| framework/src/Volo.Abp.UI.Navigation/Volo.Abp.UI.Navigation.csproj | Adds project reference to Volo.Abp.Features. |
| framework/src/Volo.Abp.Features/Volo/Abp/Features/RequireFeaturesSimpleBatchStateCheckerModel.cs | New model type to capture state + required features for batch evaluation. |
| framework/src/Volo.Abp.Features/Volo/Abp/Features/RequireFeaturesSimpleBatchStateChecker.cs | New batch state checker that evaluates feature requirements across many states using IFeatureChecker. |
| framework/src/Volo.Abp.Features/Volo/Abp/Features/IFeatureChecker.cs | Adds batch IsEnabledAsync(string[] names) API. |
| framework/src/Volo.Abp.Features/Volo/Abp/Features/FeaturesSimpleStateCheckerSerializerContributor.cs | Minor formatting/whitespace adjustments. |
| framework/src/Volo.Abp.Features/Volo/Abp/Features/FeatureSimpleStateCheckerExtensions.cs | Routes RequireFeatures(...) through batch-check path by default and adds a batchCheck overload. |
| framework/src/Volo.Abp.Features/Volo/Abp/Features/FeatureCheckerBase.cs | Provides default implementation for new batch IFeatureChecker.IsEnabledAsync(string[] names). |
| framework/src/Volo.Abp.Core/Volo/Abp/SimpleStateChecking/SimpleStateCheckerManager.cs | Reuses a single scope for batch evaluation and uses per-state transient cached providers for non-batch checkers. |
| framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/RequirePermissionsSimpleBatchStateChecker.cs | Optimizes permission aggregation/lookup for batch permission checks. |
| framework/src/Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared/Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared.csproj | Adds project reference to Volo.Abp.Features. |
| framework/src/Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared/Toolbars/ToolbarManager.cs | Wraps toolbar configuration in RequireFeaturesSimpleBatchStateChecker.Use(...). |
| framework/src/Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared/AbpAspNetCoreMvcUiThemeSharedModule.cs | Adds AbpFeaturesModule dependency so feature checking is available in theme shared module. |
Comment on lines
+14
to
+16
| public static RequireFeaturesSimpleBatchStateChecker<TState> Current => _current.Value!; | ||
| private static readonly AsyncLocal<RequireFeaturesSimpleBatchStateChecker<TState>> _current = new(); | ||
|
|
There was a problem hiding this comment.
Current => _current.Value! relies on the static ctor having set an AsyncLocal value in the current ExecutionContext. AsyncLocal values don’t automatically exist in new/isolated execution contexts, so Current can be null and cause NREs when RequireFeatures(...) calls Current.AddCheckModels(...). Consider lazily initializing Current (e.g., Value ??= new ...) and removing the static ctor initialization dependency.
| } | ||
|
|
||
| public static IDisposable Use(RequireFeaturesSimpleBatchStateChecker<TState> checker) | ||
| { |
There was a problem hiding this comment.
Use(...) accepts a potentially null checker and assigns it to the ambient AsyncLocal, which can lead to hard-to-diagnose null reference issues later. Add a null check (or Check.NotNull) for the checker parameter before assigning it.
Suggested change
| { | |
| { | |
| Check.NotNull(checker, nameof(checker)); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR generated automatically to merge dev with rel-10.3. Please review the changed files before merging to prevent any errors that may occur.